Conversation
WalkthroughThis set of changes enhances the database abstraction layer to fully support schemas with composite or custom primary keys. The codebase replaces hardcoded assumptions of a single Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Repo
participant Schema
participant Query
Client->>Repo: reload(schema_instance)
Repo->>Schema: schema.__primary_keys__()
alt No primary keys
Repo->>Client: raise error "No primary keys defined"
else Primary keys exist
Repo->>Query: Generate SQL using primary keys
Query->>Repo: Return result
Repo->>Client: Return reloaded instance
end
sequenceDiagram
participant Client
participant Query
participant Schema
Client->>Query: get_templated_query_id(query_type, table, meta)
Query->>Schema: get_model_from_query_id(query_id)
alt No primary keys
Query->>Client: raise error "Cannot generate query without primary keys"
else Primary keys exist
Query->>Query: generate SQL with dynamic WHERE clause
Query->>Client: Return query ID and metadata
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
🔇 Additional comments (18)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
lib/feeb/db/query.ex (1)
84-107: 🛠️ Refactor suggestionGuard against updates that accidentally modify primary-key columns
If a caller passes a PK in
target_fields, the generated SQL will set and filter on the same column, duplicating bind parameters and (worse) allowing PK mutation. Reject such input early:- target_fields = Enum.sort(target_fields) + primary_keys = model.__primary_keys__() + + # Prevent accidental modifications of PKs + if Enum.any?(target_fields, &(&1 in primary_keys)) do + raise ArgumentError, + "UPDATE cannot target primary keys (got: #{inspect(target_fields)}, " <> + "primary keys: #{inspect(primary_keys)})" + end + + target_fields = Enum.sort(target_fields)
🧹 Nitpick comments (3)
lib/feeb/db/query.ex (2)
256-266: Nit – duplicated leading space whenWHEREis appendedAll call-sites already prepend a space (
" … #{generate_where_clause()}").
generate_where_clause/1could omit the initial space to avoid the double-space ("SET … WHERE"). Optional, purely cosmetic.- "WHERE #{where_conditions}" + "WHERE #{where_conditions}"
268-273: Extend PK presence check to handle[]as “no PKs”Schemas can explicitly set
@primary_keys [], which currently slips through and produces invalidWHEREclauses. Re-use the same error path used fornil.-defp assert_adhoc_query!(nil, query_id, model) do +defp assert_adhoc_query!(pk, query_id, model) when pk in [nil, []] dotest/db/query_test.exs (1)
267-276: Consider parametrising cache keys instead of manual erasureHard-coding every domain risks future tests forgetting to purge new entries.
A helper that enumerates:persistent_term.get/0 |> :persistent_term.list/0and deletes keys under{:db_sql_queries, _}would scale better.Optional improvement; no immediate action required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/feeb/db.ex(1 hunks)lib/feeb/db/boot.ex(0 hunks)lib/feeb/db/query.ex(7 hunks)lib/feeb/db/repo.ex(1 hunks)lib/feeb/db/schema.ex(3 hunks)priv/test/feebdb_schemas.json(1 hunks)priv/test/migrations/test/250426163848_order_items.sql(1 hunks)test/db/boot_test.exs(1 hunks)test/db/db_test.exs(6 hunks)test/db/query_test.exs(3 hunks)test/support/db/schemas/all_types.ex(1 hunks)test/support/db/schemas/order_items.ex(1 hunks)
💤 Files with no reviewable changes (1)
- lib/feeb/db/boot.ex
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/feeb/db/repo.ex (1)
lib/feeb/db/schema.ex (1)
from_row(272-318)
test/db/db_test.exs (3)
lib/feeb/db.ex (7)
begin(24-32)insert!(154-157)update(171-175)delete(205-209)all(135-137)all(139-146)reload(239-251)lib/feeb/db/sqlite.ex (2)
one(59-66)all(55-57)test/support/db/schemas/friend.ex (2)
new(17-21)update(23-26)
🔇 Additional comments (20)
priv/test/feebdb_schemas.json (1)
1-1: Addition of OrderItems schema looks good.The schema list now includes the new
Elixir.Sample.OrderItemsschema which has been created to support composite primary keys testing.test/support/db/schemas/all_types.ex (1)
8-8: Good addition of explicit nil primary key.Setting
@primary_keys nilexplicitly marks this schema as having no primary keys, which is a good test case for the new architecture supporting both composite PKs and schemas without PKs.priv/test/migrations/test/250426163848_order_items.sql (1)
1-9: Well-structured composite PK table definition.This SQL migration creates a table with a composite primary key (order_id, product_id) which is perfect for testing the new composite primary key support. The STRICT enforcement is also a good practice for SQLite tables.
lib/feeb/db/repo.ex (4)
348-348: Good refactoring to centralize model retrieval.Replaced the private function with
Schema.get_model_from_query_id/1, centralizing the model retrieval logic in the Schema module as part of supporting composite primary keys.
353-353: Consistent refactoring pattern applied.Model retrieval logic consistently refactored to use the Schema module's implementation for insert operations.
358-358: Consistent refactoring pattern maintained.Update operations now also use the centralized model retrieval, maintaining consistent architecture throughout the codebase.
363-363: Refactoring completed for all query types.Delete operations also use the centralized model retrieval, completing the refactoring across all database operation types.
test/db/boot_test.exs (1)
27-27: Version update aligns with new migration for composite primary keys.The version change corresponds to the new migration that adds the
order_itemstable with composite primary keys.lib/feeb/db.ex (2)
240-244: Good enhancement to support schemas with composite or no primary keys.The code now dynamically retrieves primary keys and adds proper error handling when none exist, which is essential for the composite PK support.
246-248: Properly maps multiple primary key values to bindings.This change correctly maps over all primary keys to fetch their corresponding values from the struct, supporting the reload of entities with composite keys.
test/support/db/schemas/order_items.ex (3)
1-7: Well-structured schema with explicit composite primary keys.The schema correctly defines the context, table name, and explicitly sets composite primary keys.
9-16: Schema fields are well defined with appropriate types.The schema includes all necessary fields with proper types, including the composite primary keys and timestamp fields with UTC datetime types.
18-27: Standard schema operations implemented properly.The
new/1andupdate/2functions follow the established pattern in the codebase for creating and updating schema instances.test/db/db_test.exs (5)
5-5: Added OrderItems to test aliases.Appropriate inclusion of the new OrderItems schema in the test module's aliases.
409-430: Good test coverage for fetch queries with different PK types.These tests verify that the
:fetchtemplated query works correctly with both simple schemas (single PK) and schemas with composite primary keys. The test cases cover successful retrieval and non-existent records.
586-708: Comprehensive tests for CRUD operations with composite PKs.These tests thoroughly verify that insert, update, and delete operations work correctly with the OrderItems schema that has composite primary keys. All important scenarios are covered including:
- Creation of new records
- Updating existing records
- Deletion of records
- Verifying the operations succeed through subsequent queries
793-809: Good test for reloading schemas with composite PKs.This test verifies that the reload function correctly handles schemas with composite primary keys, ensuring that updates are properly reflected after reloading.
879-893: Proper error handling for schemas without PKs.This test correctly verifies that attempting to reload a schema without primary keys raises an appropriate runtime error with a clear message.
lib/feeb/db/schema.ex (1)
114-115: Nice touch – public accessor for PKsExposing
__primary_keys__/0will considerably simplify PK-introspection across the codebase.
👍 Nothing to change here.test/db/query_test.exs (1)
16-20: 👍 Good call clearing persistent-term caches insetup/0Ensures deterministic tests despite global storage. No action needed.
renatomassaro
force-pushed
the
add-support-for-composite-PKs
branch
from
d3aa895 to
8692009
Compare
1 participant
Now "templated" (adhoc) queries (and
DB.reload/1) support composite PKs.Summary by CodeRabbit
New Features
order_itemswith multiple primary key columns.OrderItems, including relevant fields and composite primary keys.Bug Fixes
Tests
Chores